RDKB-66347: Add conditional support for rdk logger init for RDKB - #102
RDKB-66347: Add conditional support for rdk logger init for RDKB#102gomathishankar37 wants to merge 6 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a compile-time switch to choose between the newer rdk_logger_ext_init() path and a legacy rdk_logger_init("/etc/debug.ini") path when RDK_LOGGER is enabled, supporting RDKB environments that still require the legacy init.
Changes:
- Add
USE_LEGACY_LOGGER_INITconditional to select logger initialization method underRDK_LOGGER. - Introduce legacy init branch calling
rdk_logger_init(DEBUG_INI_NAME).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (1)
utils/rdkv_cdl_log_wrapper.c:30
printf("RDKLOG init completed")currently runs before the logger is initialized, and it prints even whenRDK_LOGGERis not enabled (so no init happens). This makes the message misleading.
Move the message into the #if defined(RDK_LOGGER) block after whichever init path runs (or adjust wording to reflect that init may be skipped).
int log_init( ) {
printf("RDKLOG init completed\n");
#if defined(RDK_LOGGER)
#if !defined(USE_LEGACY_LOGGER_INIT)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (1)
utils/rdkv_cdl_log_wrapper.c:40
printf("RDKLOG init completed")is executed even whenRDK_LOGGERis not defined, so the message can claim initialization succeeded when the logger init path was compiled out. Consider printing the success message only when initialization actually ran (and optionally logging that init was skipped otherwise).
printf("RDKLOG init completed\n");
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
utils/rdkv_cdl_log_wrapper.h:31
get_common_util_identifier()is defined in a header; in C this should bestatic inline(and ideally use avoidparameter list) to avoid needing an external definition / causing duplicate external symbols across translation units.
inline char* get_common_util_identifier()
| #include <stdio.h> | ||
| #include <stdlib.h> | ||
|
|
||
| char* RDK_LOGGER_SHARED_NAME_IDENTIFIER_INTERNAL = NULL; |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (4)
utils/rdkv_cdl_log_wrapper.c:27
- To match the header and improve const-correctness, define this as
const char *(pointer can still be reassigned, but the referenced string isn’t meant to be mutated).
char* RDK_LOGGER_SHARED_NAME_IDENTIFIER_INTERNAL = NULL;
utils/rdkv_cdl_log_wrapper.h:32
- For const-correctness and to avoid accidental modification, this identifier and accessor can be
const char *(the returned buffer is internally managed and should not be mutated by callers).
extern char* RDK_LOGGER_SHARED_NAME_IDENTIFIER_INTERNAL;
#define RDK_LOGGER_PREFIX "LOG.RDK"
#define COMMON_UTIL_IDENTIFIER ".COMMONUTILITIES"
#define IDENTIFIER_LEN 256
static inline char* get_common_util_identifier(void)
{
utils/rdkv_cdl_log_wrapper.h:45
get_common_util_identifier()lazily initializes a shared static buffer without synchronization. If two threads log concurrently before the first initialization completes, this introduces a data race (undefined behavior). Also, because the value is cached after the first call, later changes toRDK_LOGGER_SHARED_NAME_IDENTIFIER_INTERNALwill never be reflected.
static char logger_identifier[IDENTIFIER_LEN] = {0};
if (logger_identifier[0] == '\0')
{
if (RDK_LOGGER_SHARED_NAME_IDENTIFIER_INTERNAL == NULL)
{
snprintf(logger_identifier, IDENTIFIER_LEN, "%s%s", RDK_LOGGER_PREFIX, COMMON_UTIL_IDENTIFIER);
}
else
{
snprintf(logger_identifier, IDENTIFIER_LEN, "%s%s", RDK_LOGGER_SHARED_NAME_IDENTIFIER_INTERNAL, COMMON_UTIL_IDENTIFIER);
}
}
return logger_identifier;
utils/rdkv_cdl_log_wrapper.c:27
RDK_LOGGER_SHARED_NAME_IDENTIFIER_INTERNALis introduced as a global override, but within this repo it is never assigned anywhere (only defined asNULL). That means the shared-identifier branch inget_common_util_identifier()is effectively unreachable unless some external integrator sets this global; consider adding an explicit setter API (or documenting/encapsulating the override) so the behavior is controllable and less fragile.
This issue also appears on line 27 of the same file.
char* RDK_LOGGER_SHARED_NAME_IDENTIFIER_INTERNAL = NULL;
No description provided.